Skip to content

fix(modern-compiler): dispose redundant compilers - #1245

Merged
alexander-akait merged 3 commits into
webpack:masterfrom
hikiko4ern:dispose-redundant-compilers
Nov 1, 2024
Merged

fix(modern-compiler): dispose redundant compilers#1245
alexander-akait merged 3 commits into
webpack:masterfrom
hikiko4ern:dispose-redundant-compilers

Conversation

@hikiko4ern

Copy link
Copy Markdown
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

idk how to cover this by tests

Motivation / Use-Case

fixes #1244

Breaking Changes

No

Additional Info

No

@linux-foundation-easycla

linux-foundation-easycla Bot commented Oct 31, 2024

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

@evenstensberg evenstensberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you sign the CLA?

@hikiko4ern

Copy link
Copy Markdown
Contributor Author

Sure, signed it.

evenstensberg
evenstensberg previously approved these changes Oct 31, 2024
Comment thread src/utils.js
compiler.dispose();
});
} else {
compiler.dispose();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of if/elses deeply nested, but we can't return early in this scope, so it's okay.

@codecov

codecov Bot commented Oct 31, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.23%. Comparing base (31789cc) to head (6ad4ecb).
Report is 30 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1245      +/-   ##
==========================================
- Coverage   94.44%   94.23%   -0.22%     
==========================================
  Files           3        3              
  Lines         360      416      +56     
  Branches      132      158      +26     
==========================================
+ Hits          340      392      +52     
- Misses         18       22       +4     
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@evenstensberg

Copy link
Copy Markdown
Member

Seems like the coverage is failing here. Could you add a test for this?

@alexander-akait alexander-akait left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong

@alexander-akait alexander-akait left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a test case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extra sass-embedded compilers are not disposed

3 participants